Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pylint] unspecified-encoding (W1514) #3416

Closed
wants to merge 4 commits into from
Closed

Conversation

csko
Copy link

@csko csko commented Mar 9, 2023

Partially addresses #3278

Implements the core features for w1514, including having "b" in the mode arg.

Not all fixtures work properly, in particular:

  • this only covers the builtin open()
  • variables are not inferred; open(x, encoding=y)
  • this does not handle class type arguments

Comment on lines +55 to +62
// If `open` has been rebound, skip this check entirely.
if !checker.ctx.is_builtin(OPEN_FUNC_NAME) {
return;
}
// Look for open().
if !matches!(&func.node, ExprKind::Name {id, ..} if id == OPEN_FUNC_NAME) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I recommend switching the is_builtin check and testing the function name because testing is_builtin is more expensive than comparing two strings

Suggested change
// If `open` has been rebound, skip this check entirely.
if !checker.ctx.is_builtin(OPEN_FUNC_NAME) {
return;
}
// Look for open().
if !matches!(&func.node, ExprKind::Name {id, ..} if id == OPEN_FUNC_NAME) {
return;
}
// Look for open().
if !matches!(&func.node, ExprKind::Name {id, ..} if id == OPEN_FUNC_NAME) {
return;
}
// If `open` has been rebound, skip this check entirely.
if !checker.ctx.is_builtin(OPEN_FUNC_NAME) {
return;
}

}

// Check encoding for missing or None values.
let encoding_arg = call_args.get_argument(ENCODING_KEYWORD_ARGUMENT, Some(3));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I recommend inlining the ENCODING_KEYWORD_ARGUMENT constant because it is only used once and it can break the reading flow if I have to jump to the top to understand what the value of the constant is.

open(FILENAME, "w", encoding="utf-8")
open(FILENAME, "wb")
open(FILENAME, "w+b")
open(FILENAME) # [unspecified-encoding]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue here is that this rule is in conflict with UP015, which removes redundant open modes (so, e.g., it changes open("foo", "r") to open("foo")). We'd have to mark these rules as incompatible in the INCOMPATIBLE_CODES slice in crates/ruff/src/registry.rs.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Mar 17, 2023
@charliermarsh
Copy link
Member

Closing to keep the pull request queue up-to-date -- we'd have to resolve some incompatibilities with existing rules here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants